[DatePicker] Don't run onChange if same date selected #1967

todor-a · 2020-07-09T13:29:38Z

For some reason the function areValuesEqual in valueManager was returning false when checking if two dates are equal (when in fact they were). This only happened the first time you choose the current value. If the picker's value was, let's say, 08/07/2020 and you selected it again, it would return false but if you select it a second time, it would return true.

To solve it, I added another function - areDaysEqual.

vercel · 2020-07-09T13:29:44Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

cypress · 2020-07-09T13:41:04Z

Test summary

78 • 0 • 3 • 0

Run details


Project	material-ui-pickers
Status	Passed
Commit	`db16c0b`
Started	Jul 14, 2020 6:45 PM
Ended	Jul 14, 2020 6:46 PM
Duration	01:38 💡
OS	Linux Debian - 10.0
Browser	Chrome 80

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

oliviertassinari

Could you add a failing test case with testing-library that demonstrate the fix? Thanks

Also, I believe we should test all the components, date picker, date time picker, time picker, etc.

todor-a · 2020-07-09T13:44:46Z

I'll get to it. Sorry, it's my first opensource contribution :)

dmtrKovalenko

I think overall logic is not correct, please add a test (with testing library) that confirms desired behavior

dmtrKovalenko · 2020-07-09T13:46:06Z

lib/src/_shared/hooks/usePickerState.ts

+        if (!valueManager.areDaysEqual(utils, newDate, pickerDate)) {
+          setPickerDate(newDate);
+        }


Suggested change

if (!valueManager.areDaysEqual(utils, newDate, pickerDate)) {

setPickerDate(newDate);

}

if (valueManager.areDaysEqual(utils, newDate, pickerDate)) {

return;

}

setPickerDate(newDate);

I might be missing something but wouldn't this run only if you select the same date?

Yes, you are right but we still doesn't need to update the state.
Another question – what for TimePicker and DateTimePicker? I think overall we need to add this check to the Calendar component.

oliviertassinari · 2020-07-13T13:11:27Z

@tdranv Did you have the time to look at the feedback from the review? :)

todor-a · 2020-07-13T18:36:35Z

I did but I'm having trouble with what you meant by "Could you add a failing test case with testing-library that demonstrate the fix?".

Can you elaborate a little more on that?

dmtrKovalenko · 2020-07-13T19:44:44Z

@tdranv firstly please make this check on the <Day /> side. We already know which day is currently selected and which will be selected. I think the perfect place for this check will be this handler:

https://github.com/mui-org/material-ui-pickers/blob/c3c423179ceae40da6f3b5212e2618af44c2b161/lib/src/views/Calendar/Day.tsx#L181-L189

So proposed diff will be this:

--- a/lib/src/views/Calendar/Day.tsx
+++ b/lib/src/views/Calendar/Day.tsx
@@ -179,7 +179,7 @@ const PureDay: React.FC<DayProps> = ({
   };
 
   const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
-    if (!disabled) {
+    if (!disabled && !selected) {
       onDaySelect(day, 'finish');
     }

And also please add a new test case to this file:
https://github.com/mui-org/material-ui-pickers/blob/e6bfd12b759445cb8341d7669b2290412da93153/lib/src/__tests__/DatePickerTestingLib.test.tsx#L8

oliviertassinari · 2020-07-13T23:13:13Z

Can you elaborate a little more on that?

@tdranv Sure, the idea is that for a test to be relevant, it has to fail without the fix, otherwise we are testing something that already works.

…ckers into issue-1652

todor-a · 2020-07-14T09:10:50Z

I seem to have broken something else. I will take a look at it later. Thanks for being patient with me. :)

dmtrKovalenko · 2020-07-14T09:15:35Z

From what I am seeing it looks like this behavior is not suitable for DateRangePicker. We must have the ability to select the same day for start and end. (#1759)

What I propose: add new prop allowSameDateSelection for the <Day /> component. This will allow to configure this behavior for the users thanks to renderDay and also we will pass allowSameDateSelection={true} in the DateRangeDay.

UPD: thanks so much for your work ❤️

oliviertassinari · 2020-07-14T09:57:40Z

@dmtrKovalenko Regarding #1759, what about we solve it with #1759 (comment)?

For the date range picker, I think that the equivalent fix would be that if the same range is selected, no change event is fired.

todor-a · 2020-07-14T17:46:54Z

I added the flag you suggested @dmtrKovalenko.

First, once again, thank you for being patient with me. I like Material UI a lot and wish to be part of it :) Here are the issues are encountered, as a first-time contributor:

I find it hard to configure the development environment. Every time I run the tests I get all these errors, saying that Your test suite must contain at least one test. or Test suite failed to run. I assume that this is due to something I misconfigured but it would be useful to go into a little more detail on how to set up your environment in the How to Contribute file.
I also couldn't run the linter/prettier.
I need some more time to get acquainted with how the project is structured (which was surprisingly easy for me!)

Anyway, working on this (despite it being such a small issue) is fun! Thanks for your guidance :)

lib/src/__tests__/DatePickerTestingLib.test.tsx

lib/src/views/Calendar/Day.tsx

lib/src/__tests__/DatePickerTestingLib.test.tsx

oliviertassinari · 2020-07-14T18:24:12Z

lib/src/__tests__/DatePickerTestingLib.test.tsx

+      />
+    );
+
+    fireEvent.click(screen.getByLabelText('Choose date, selected date is Jan 1, 2018'));


I side note, I wonder if the asserting here isn't too brittle. What happens if we change the formation or the value? Would it be possible to query an element with role button instead?

I too think that this approach to querying can be brittle. But also fixing it should probably part of a separate issue/pull request?

Yeah, right, a different pull request would make sense

lib/src/__tests__/DatePickerTestingLib.test.tsx

oliviertassinari

Looks solid

oliviertassinari · 2020-07-14T18:30:05Z

lib/src/views/Calendar/Day.tsx

@@ -123,6 +123,11 @@ export interface DayProps extends ExtendMui<ButtonBaseProps> {
   * @default false
   */
  disableHighlightToday?: boolean;
+  /**
+   * Allow selecting the same date (fire onChange even if same date is selected).
+   * @default false


@eps1lon This change makes me wonder. On the main repository, we don't mention the default value in the TypeScript definitions, we have a tool that extracts them from the source.

Here, the upside is that we get the information right in the editor:

But it can get quickly out-of-sync. What do you think about the tradeoff?

Great idea. I think we could statically verify if the default value matches. Though I wouldn't worry about it too much at first. In the end default values will rarely change in SemVer patches or minors because they're very likely breaking changes.

I don't remember how we extract default values but if we extract them statically then we can also match them against TS types.

dmtrKovalenko

@tdrandv Thanks, everything looks nice except this commit. Can you please revert it and we could merge it 💪

dmtrKovalenko · 2020-07-15T06:58:07Z

docs/pages/guides/css-overrides.mdx

+Add the following to a TypeScript declaration file in your project, such as `overrides-mui.d.ts`:
+


Suggested change

Add the following to a TypeScript declaration file in your project, such as `overrides-mui.d.ts`:

Ops. Should be good now :)

Requested changes were done

oliviertassinari · 2020-07-15T13:14:56Z

@tdranv For the tests, do you think that it would make sense to cover the other components, TimePicker, DateRangePicker, DateTimePicker?

todor-a · 2020-07-15T17:58:41Z

Hm, I believe no for TimePicker. As far as DateRangePicker, a user can select the same date if the start and end dates are the same, as @dmtrKovalenko pointed out in a previous comment.

oliviertassinari · 2020-07-15T18:27:57Z

What should happen if a end-user uses the date range picker and choose the same range than previously? Should it fire a change event or not?

todor-a · 2020-07-15T20:54:08Z

As far as DateRangePicker...

I probably did not say that correctly. What I mean was that a end-user can pick the same date for the start and end date. As far as your question: I believe it should not fire the event.

oliviertassinari · 2020-07-16T10:09:46Z

@tdranv Ok cool. It was a great first pull request on Material-UI 👌🏻. Thank you for working on it!

oliviertassinari · 2020-07-16T10:15:57Z

@tdranv If you would like to keep helping without diving too deep in the complexity of the components, there is ongoing work around unifying the codebase.

For instance, mui/material-ui#21758 was recently merged on the main repository, it would be perfect if we could enforce the exact same eslint configuration here. I have started a couple of iterations already (doing it in small chunks), I'm doing that for an hour or two during the weekend. But if you want to speed things up, feel free too. Related to #1930.

todor-a · 2020-07-16T15:31:08Z

Sure. I will have a look at it over the weekend. :)

Don't run onChange if same date selected

f9fe935

vercel bot deployed to Preview July 9, 2020 13:29 View deployment

oliviertassinari previously requested changes Jul 9, 2020

View reviewed changes

dmtrKovalenko requested changes Jul 9, 2020

View reviewed changes

oliviertassinari added the bug 🐛 Something isn't working label Jul 9, 2020

todor-a and others added 2 commits July 14, 2020 10:43

Add test for selecting same date

4ab0732

Merge branch 'next' into issue-1652

9f61e10

vercel bot temporarily deployed to Preview July 14, 2020 07:45 Inactive

Merge branch 'issue-1652' of https://github.com/tdranv/material-ui-pi…

6c923dd

…ckers into issue-1652

vercel bot deployed to Preview July 14, 2020 07:48 View deployment

Add allowSameDateSelection

1e3528a

vercel bot deployed to Preview July 14, 2020 17:38 View deployment

Change onChange not called test

b9aa6cc

vercel bot deployed to Preview July 14, 2020 18:04 View deployment

oliviertassinari reviewed Jul 14, 2020

View reviewed changes

oliviertassinari added the component: DatePicker label Jul 14, 2020

oliviertassinari changed the title ~~Don't run onChange if same date selected~~ [DatePicker] Don't run onChange if same date selected Jul 14, 2020

Address PR comments.

db16c0b

vercel bot deployed to Preview July 14, 2020 18:41 View deployment

vercel bot had a problem deploying to Preview July 15, 2020 06:07 Failure

dmtrKovalenko reviewed Jul 15, 2020

View reviewed changes

todor-a force-pushed the issue-1652 branch from 0a60e63 to db16c0b Compare July 15, 2020 07:47

dmtrKovalenko approved these changes Jul 15, 2020

View reviewed changes

dmtrKovalenko merged commit 6b0d30b into mui:next Jul 15, 2020

todor-a deleted the issue-1652 branch July 15, 2020 11:29

todor-a added a commit to todor-a/material-ui-pickers that referenced this pull request Jul 18, 2020

[DatePicker] Don't run onChange if same date selected (mui#1967)

61be87e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DatePicker] Don't run onChange if same date selected #1967

[DatePicker] Don't run onChange if same date selected #1967

todor-a commented Jul 9, 2020

vercel bot commented Jul 9, 2020 •

edited

Loading

cypress bot commented Jul 9, 2020 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

todor-a commented Jul 9, 2020

dmtrKovalenko left a comment

dmtrKovalenko Jul 9, 2020 •

edited

Loading

todor-a Jul 9, 2020

dmtrKovalenko Jul 9, 2020

oliviertassinari commented Jul 13, 2020

todor-a commented Jul 13, 2020

dmtrKovalenko commented Jul 13, 2020

oliviertassinari commented Jul 13, 2020

todor-a commented Jul 14, 2020

dmtrKovalenko commented Jul 14, 2020 •

edited

Loading

oliviertassinari commented Jul 14, 2020 •

edited

Loading

todor-a commented Jul 14, 2020

oliviertassinari Jul 14, 2020

todor-a Jul 14, 2020

oliviertassinari Jul 15, 2020

oliviertassinari left a comment

oliviertassinari Jul 14, 2020 •

edited

Loading

eps1lon Jul 14, 2020

dmtrKovalenko left a comment

dmtrKovalenko Jul 15, 2020

todor-a Jul 15, 2020

oliviertassinari commented Jul 15, 2020

todor-a commented Jul 15, 2020

oliviertassinari commented Jul 15, 2020 •

edited

Loading

todor-a commented Jul 15, 2020

oliviertassinari commented Jul 16, 2020

oliviertassinari commented Jul 16, 2020 •

edited

Loading

todor-a commented Jul 16, 2020

		Add the following to a TypeScript declaration file in your project, such as `overrides-mui.d.ts`:

[DatePicker] Don't run onChange if same date selected #1967

[DatePicker] Don't run onChange if same date selected #1967

Conversation

todor-a commented Jul 9, 2020

vercel bot commented Jul 9, 2020 • edited Loading

cypress bot commented Jul 9, 2020 • edited Loading

Test summary

Run details

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

todor-a commented Jul 9, 2020

dmtrKovalenko left a comment

Choose a reason for hiding this comment

dmtrKovalenko Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jul 13, 2020

todor-a commented Jul 13, 2020

dmtrKovalenko commented Jul 13, 2020

oliviertassinari commented Jul 13, 2020

todor-a commented Jul 14, 2020

dmtrKovalenko commented Jul 14, 2020 • edited Loading

oliviertassinari commented Jul 14, 2020 • edited Loading

todor-a commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jul 15, 2020

todor-a commented Jul 15, 2020

oliviertassinari commented Jul 15, 2020 • edited Loading

todor-a commented Jul 15, 2020

oliviertassinari commented Jul 16, 2020

oliviertassinari commented Jul 16, 2020 • edited Loading

todor-a commented Jul 16, 2020

vercel bot commented Jul 9, 2020 •

edited

Loading

cypress bot commented Jul 9, 2020 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

dmtrKovalenko Jul 9, 2020 •

edited

Loading

dmtrKovalenko commented Jul 14, 2020 •

edited

Loading

oliviertassinari commented Jul 14, 2020 •

edited

Loading

oliviertassinari Jul 14, 2020 •

edited

Loading

oliviertassinari commented Jul 15, 2020 •

edited

Loading

oliviertassinari commented Jul 16, 2020 •

edited

Loading